Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tidy PersistableEnvelope inheritance #4102

Merged

Conversation

stejbac
Copy link
Contributor

@stejbac stejbac commented Mar 30, 2020

This refactoring is a continuation from the concurrency fixes in #4025, to remove all the remaining superfluous PersistableEnvelope interfaces on classes implementing Proto.

  • Remove an unused PersistableEnvelope interface from the following five PersistableNetworkPayload implementations:

    AccountAgeWitness, BlindVotePayload, ProposalPayload, SignedWitness, TradeStatistics2

    These already have corresponding *Store envelope classes which correctly implement the interface.

  • Additionally, delete the unused class PersistableHashMap, make (UserThreadMapped)PersistableList abstract and remove some unused functionality from the base class.

  • The last commit in this PR touches the DAO packages, as the consensus-critical classes MeritList and VoteWithProposalTxIdList needlessly implement the PersistableEnvelope interface prior to this change, via the base class PersistableList. I inlined the list field, instead of inheriting it from the base class, and made them standalone classes instead. This shouldn't observably alter behaviour and would hopefully also reduce the chance of future inadvertent changes in behaviour (as PersistableList lies outside the DAO packages).

stejbac added 3 commits March 28, 2020 15:22
Remove an unused PersistableEnvelope interface from the following five
PersistableNetworkPayload implementations:

  AccountAgeWitness, BlindVotePayload, ProposalPayload,
  SignedWitness, TradeStatistics2

These already have corresponding *Store envelope classes which correctly
implement the interface.
Remove unused functionality to pass a custom 'toProto' function via a
constructor or setter, as it was unused and all subclasses override the
toProtoMessage() method anyway. In this way, the method may be removed
and the class (+ UserThreadMappedPersistableList) made abstract.

Also it appears PersistableHashMap was never used, so remove it.
Remove an unnecessary PersistableEnvelope interface by making them
standalone @value classes with private List fields, instead of extending
PersistableList. As they weren't using any functionality of the latter
other than the getList() and stream() methods, this should not alter
behaviour, outside MeritList::toString.

Also comment out the MERIT_LIST PersistableEnvelope protobuf message
type, which shouldn't be encountered as merit lists were never persisted
directly to a storage file.

This removes the last superfluous PersistableEnvelope implementations,
leaving the following type hierarchy:

  PersistableEnvelope *
  +- NavigationPath
  +- PeerList
  +- PersistableList *
  +- ThreadedPersistableEnvelope *
  |  +- AccountAgeWitnessStore
  |  +- BlindVoteStore
  |  +- DaoStateStore
  |  +- PersistableNetworkPayloadList              *  is abstract
  |  +- ProposalStore
  |  +- SequenceNumberMap
  |  +- SignedWitnessStore
  |  +- TempProposalStore
  |  \- TradeStatistics2Store
  \- UserThreadMappedPersistableEnvelope *
     +- AddressEntryList
     +- DisputeList *
     |  +- ArbitrationDisputeList
     |  +- MediationDisputeList
     |  \- RefundDisputeList
     +- UserThreadMappedPersistableList *
     |  +- BallotList
     |  +- MyBlindVoteList
     |  +- MyProofOfBurnList
     |  +- MyProposalList
     |  +- MyReputationList
     |  +- MyVoteList
     |  +- PaymentAccountList
     |  \- UnconfirmedBsqChangeOutputList
     +- PreferencesPayload
     +- TradableList
     \- UserPayload
@stejbac stejbac changed the title Tidy persistable envelope inheritance Tidy PersistableEnvelope inheritance Mar 30, 2020
@ripcurlx ripcurlx requested a review from ManfredKarrer March 30, 2020 17:49
@ripcurlx ripcurlx added this to the v1.3.0 milestone Apr 1, 2020
Copy link
Member

@sqrrm sqrrm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the first two commits, looks good to me but I haven't tested.

The third commit, touching the DAO classes makes me worried. I think it's fine and that it won't cause any trouble for the consensus but I'm not confident. Considering the possible trouble I would hesitate to merge without more thorough review and testing. We also still need the review of @ManfredKarrer

@ripcurlx
Copy link
Contributor

ripcurlx commented Apr 2, 2020

We also still need the review of @ManfredKarrer

Just pinged him, so we get this into master for the next release.

Copy link
Contributor

@ManfredKarrer ManfredKarrer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

I recommend to test a few cycles in regtest where the changed classes are used to see if it does not break consensus. Also maybe better to merge it after the next release so we get more time for dev testing to detect potential issues. I dont see any but the DAO consensus is tricky.

@ripcurlx ripcurlx removed this from the v1.3.0 milestone Apr 3, 2020
@ripcurlx ripcurlx added the is:priority PR or issue marked with this label is up for compensation label Apr 10, 2020
@ripcurlx ripcurlx added this to the v1.3.3 milestone Apr 10, 2020
@ripcurlx
Copy link
Contributor

Just tested a couple of cycles on Regtest.
The only thing I recognized was, that although the consensus hashes were fine, the voting results were only shown for one party. After a restart the results were shown correctly to the other party as well.

Before restart
Bildschirmfoto 2020-04-27 um 15 50 26
After restart
Bildschirmfoto 2020-04-27 um 15 51 25

@stejbac Do you think this can be caused by this changes? Could be an existing bug, that I just never recognized during testing.

@ripcurlx
Copy link
Contributor

Just tested a couple of cycles on Regtest.
The only thing I recognized was, that although the consensus hashes were fine, the voting results were only shown for one party. After a restart the results were shown correctly to the other party as well.

Before restart
Bildschirmfoto 2020-04-27 um 15 50 26
After restart
Bildschirmfoto 2020-04-27 um 15 51 25

@stejbac Do you think this can be caused by this changes? Could be an existing bug, that I just never recognized during testing.

As this just happened on Mainnet as well, I have no concerns about this anymore 😬

Copy link
Contributor

@ripcurlx ripcurlx left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is:priority PR or issue marked with this label is up for compensation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants